feat(benchmarks): add unified benchmark runner#326
Conversation
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — c78df7c9
This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.
tangletools · auto-approval · reason: drewstone_author · 2026-07-08T01:08:53Z
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 4 (1 medium-concern, 3 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 187.2s (2 bridge agents) |
| Total | 187.2s |
💰 Value — sound-with-nits
Adds a campaign-backed runner, metric calibration, and standard retrieval-format helpers for BenchmarkAdapter implementations, fitting the existing substrate grain; a few minor naming and helper-duplication nits.
- What it does: Adds a unified benchmark runner (
runBenchmarkAdapter) that wires anyBenchmarkAdapterinto the existingrunCampaigninfrastructure, produces JSON/Markdown reports with split/tag/dimension summaries and cost/latency distributions, adds a weak/strong metric calibration helper (calibrateBenchmarkMetric), and adds standard retrieval format parsers/evaluators (BEIR/MTEB/MS MARCO/TREC/MIRACL-sty - Goals it achieves: Provides a single substrate primitive for running objective benchmarks against deterministic splits, scoring responses, tracking cost/latency, and persisting standardized reports. This lets teams add new retrieval or classification benchmarks without reimplementing campaign orchestration, report rendering, or metric math, and gives the package a first-party smoke benchmark (routing) that exercises
- Assessment: Good on merits. The change is coherent with the codebase: it reuses
runCampaign,CampaignStorage,JudgeConfig,deterministicSplit, andinMemoryCampaignStorage; it follows the package's fail-loud philosophy (e.g., dataset loaders throw clear setup errors); tests cover the new runner, calibration, and retrieval scoring. The new metadata fields onBenchmarkAdaptermake the contract more s - Better / existing approach: No materially better architecture — the campaign-backed runner is the right approach. Two minor improvements: (1) the new
BenchmarkReportinterface insrc/benchmarks/runner.tscollides with the legacyBenchmarkReportinsrc/types.ts(used byBenchmarkRunner/formatBenchmarkReport); rename it (e.g.,BenchmarkRunReport) to avoid package-level confusion. (2) The localdistribution/`per - Model: opencode/kimi-for-coding/k2p7
- Bridge attempts: 1
🎯 Usefulness — sound-with-nits
Clean, well-integrated runner that bridges the existing BenchmarkAdapter contract to the existing runCampaign orchestrator and fills a real retrieval-metrics gap; only minor naming-overlap with the legacy BenchmarkRunner.
- Integration: Reachable via
export * as benchmarksat src/index.ts:1179.runBenchmarkAdapter(src/benchmarks/runner.ts:89) wrapsrunCampaign(src/campaign/run-campaign.ts:128) with a correctly-shaped JudgeConfig (src/benchmarks/runner.ts:131) — type signatures for JudgeConfig/JudgeScore/Scenario/CampaignCellResult/CampaignResult all verified against src/campaign/types.ts. The routing adapter already imple - Fit with existing patterns: Fits the codebase grain precisely: it is the missing orchestrator for the
BenchmarkAdaptercontract that already lived insrc/benchmarks/types.ts:81. It reusesrunCampaign(resumability, manifest hash, cost ceiling, concurrency, expectUsage) rather than reinventing it, and persists reports through the sameCampaignStorageabstraction (src/campaign/storage.ts) that the campaign spine uses. - Real-world viability: Concurrency, error paths, and cost ceilings are inherited from
runCampaign(maxConcurrency lanes, per-cell timeout/abort, costCeilingReached short-circuit). Failed cells are excluded from thesuccessfulrows before distribution/slice aggregation and surfaced viacellsFailed(src/benchmarks/runner.ts:227).distribution()andmean()guard against empty/NaN inputs (return zeros). `firstJudg - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Local distribution helpers duplicate existing repo patterns [duplication] ``
src/benchmarks/runner.ts:343-369 defines its own
distribution,percentile,mean, andclamp01helpers. The codebase already has many local copies of quantile/percentile/median helpers (src/release-confidence.ts:564, src/matrix/aggregation.ts:24, src/fuzz/cube.ts:47, src/rl/dataset.ts:109, src/judge-calibration.ts:521). None of the existing helpers exactly match theBenchmarkDistributionshape (n/min/mean/median/p90/max), so reuse is not trivial, but this is another copy of a common patt
🟠 BenchmarkReport name collides with legacy BenchmarkReport [better-architecture] ``
src/benchmarks/runner.ts:39 defines
BenchmarkReport, but src/types.ts:107 already exports a differentBenchmarkReportused byBenchmarkRunnerandformatBenchmarkReport. Both are exported from the package (the legacy one from@tangle-network/agent-eval, the new one from@tangle-network/agent-eval/benchmarks). Rename the new one (e.g.,BenchmarkRunReportorBenchmarkAdapterReport) to avoid import-time confusion and maintenance mistakes.
🟡 Existing example adapters lack new metadata fields [against-grain] ``
The PR adds
id,family,taskKind,description, anddefaultMetrictoBenchmarkAdapterand updatessrc/benchmarks/routing/index.ts, but the reference implementations inexamples/benchmarks/gsm8k/index.tsandexamples/benchmarks/swebench-lite/index.tsdo not set them. This is inconsistent; consumers copying those examples will produce adapters with missing metadata, so reports will fall back to'custom'and lose benchmark identity.
🎯 Usefulness Audit
🟡 Three overlapping 'benchmark runner' surfaces may briefly confuse consumers [problem-fit] ``
The repo now has (1)
BenchmarkRunnerclass in src/benchmark.ts (legacy multi-turn agent scenarios via TCloud), (2)runBenchmarkAdapterin src/benchmarks/runner.ts (dataset-backed adapter orchestration via runCampaign), and (3)product-benchmark/export helpers. Each has a distinct job and lives in a distinct namespace/path, so this is NOT a duplication — but the naming overlap (two things called 'benchmark runner') could send a new consumer to the wrong surface. Not gating; consider a one-
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
What changed
Proof